Add solution for BinarySearchTree #11

Showndarya · 2018-04-08T07:02:13Z

Solution for the BinaryTree problem in Session8.
Answer for question in BinarySearchTree.java in comments.
Comments for each function also included.

I observed that the solution for LinkedList was present already. So, didn't add that.

havanagrawal · 2018-04-12T19:09:36Z

Session08/binarytree/solutions/BinarySearchTree.java

+
+    public BinarySearchTree()
+    {
+        super(0);


Why is the root randomly 0? It will be extremely unexpected if a user constructs an "empty" binary search tree, but minValue or maxValue will return 0.

havanagrawal · 2018-04-12T19:10:23Z

Session08/binarytree/solutions/BinarySearchTree.java

+    // insert node
+	@Override
+	public boolean insert(int value) {
+		super.root = insert(super.root, value);


What if the node already exists? This function always returns true regardless of whether the insert "fails" or succeeds.

havanagrawal · 2018-04-12T19:11:56Z

Session08/binarytree/solutions/BinarySearchTree.java

+    private TreeNode insert(TreeNode root, int data)
+    {
+        // if current node is null, insert node with data
+        if (root == null) root = new TreeNode(data);


If both your if and else blocks have a single line, this formatting is fine, like the way you have used below.

However, in this case, please adhere to the repo's coding style of

if (<condition>) { } else { }

Curly braces are good. Always use them, even when they are not needed.

havanagrawal · 2018-04-12T19:12:40Z

Session08/binarytree/solutions/BinarySearchTree.java

+	@Override
+	public boolean delete(int value) {
+		super.root = deleteRec(super.root, value);
+		if(flag==1) {flag=0;return false;}


Please format as

if (flag == 1) { flag = 0; return false; }

Note the spaces

havanagrawal · 2018-04-12T19:13:42Z

Session08/binarytree/solutions/BinarySearchTree.java

+
+class BinarySearchTree extends BinaryTree {
+
+    private int flag = 0;


Any reason why this is not a boolean? Also, why is it a member variable? What does "BinarySearchTree.flag" mean for the tree as a whole?

havanagrawal · 2018-04-12T19:14:09Z

Session08/binarytree/solutions/BinarySearchTree.java

+		else return true;
+	}
+
+	TreeNode deleteRec(TreeNode root, int key)


Please follow code style of opening the curly brace on the same line.

havanagrawal · 2018-04-12T19:14:41Z

Session08/binarytree/solutions/BinarySearchTree.java

+        return root;
+    }
+
+    // delete node


Is this comment really necessary? The function name is delete. Self-documenting code.

havanagrawal · 2018-04-12T19:14:57Z

Session08/binarytree/solutions/BinarySearchTree.java

+        super(0);
+    }
+
+    // insert node


Is this comment really necessary? The function name is insert. Self-documenting code.

havanagrawal · 2018-04-12T19:15:24Z

Session08/binarytree/solutions/BinarySearchTree.java

+    }
+
+    // find the lowest value in the tree by reaching the leftmost leaf node
+    int minValue(TreeNode root)


What happens when the tree is empty?

havanagrawal · 2018-04-12T19:15:35Z

Session08/binarytree/solutions/BinarySearchTree.java

+        return minv;
+    }
+
+	// check if node exists


Is this comment really necessary? The function name is exists. Self-documenting code.

havanagrawal · 2018-04-12T19:17:23Z

Session08/binarytree/solutions/BinaryTree.java

+
+	public void inOrderTraversal() {
+		//TODO
+		inorderRec(root);


I am assuming that inOrderRec() stands for inOrderRecursive. However, there is no way for me to know this, since it's not a convention. Please name it inOrderRecursive

havanagrawal · 2018-04-12T19:19:15Z

Session08/binarytree/solutions/TreeNode.java

+	}
+
+	// set left node 
+	public void setLeft(TreeNode n)


This is really bad! We do not want setters on a TreeNode. Imagine what happens if I insert a TreeNode into a BinarySearchTree, and then set its value to be something else. Now you've violated the BinarySearchTree invariant.

Additionally, member variables without an explicit access modifier are default, and have package level access. Thus we don't really need getters either.

(In the ideal case, the TreeNode class would be immutable.)

havanagrawal · 2018-04-12T19:20:06Z

Session08/binarytree/solutions/BinarySearchTree.java

+        // traverse to right sub-tree if value to be inserted is greater than value in current node
+        else if (key > root.data) root.right = deleteRec(root.right, key);
+
+        // if value is same as current value, delte it


typo in comment, should be "delete"

havanagrawal · 2018-04-12T19:23:33Z

Session08/binarytree/solutions/BinarySearchTree.java

+        if (root == null) root = new TreeNode(data);
+        else
+        {
+            // traverse to left sub-tree if value to be inserted is lesser than value in current node


Your comment says "lesser than", but your code says "lesser than or equal to". Decide which one is incorrect.

havanagrawal · 2018-04-12T19:35:24Z

tl;dr
Hi @Showndarya , thank you for the PR. I appreciate your initiative, and it is an excellent attempt. Please see comments, and update your PR.

The long version
I see that you're a student in the C2C 2018 cohort, and once again, I admire your willingness to learn. Please do note that the above comments are something you would have learned before we reached this topic. We focus a lot on good design.

Some broad improvements you can make:

Follow the code-style of the project. Look at how the code is structured in the rest of the repo, and try to mimic it. Before you contribute to any project, it is important that you follow their code-style, rather than your personal choice.
Don't use global "flags".
Don't use an IDE (at this stage). It inserts boilerplate for you, and more often than not, you don't need it (at least not for this course). You should think long and hard before you add a public function to a class.
Contrary to popular belief, comments aren't really that great. Robert Martin (Uncle Bob) has a fantastic one-liner on this, (I paraphrase) "It is a mistake to think that a programmer who failed to express his thought in code will be able to do it in English." Avoid comments. Put them only where absolutely necessary, and then think if you can restructure your code to make the idea clearer.

If you want, go ahead and make the above changes, and I'll review again.

Cheers and happy learning 😄

havanagrawal · 2018-04-12T19:37:13Z

Session08/binarytree/solutions/BinarySearchTree.java

@@ -0,0 +1,123 @@
+// This class fails to compile, can you figure out why, and fix it?
+// This is beacuse BinaryTree has a constructor which is not default. Hence, when we extend this class and try to call a no-argument constructor, it'll give a compile-time error. so, we call it by passing the parameters (to instantiate the root node) in super() called inside the constructor of BinarySearchTree


Please use multiline comments for long comments.

/* line1 * line2 */

havanagrawal · 2018-04-12T19:39:45Z

Session08/binarytree/solutions/BinarySearchTree.java

+		else return tn;
+	}
+
+	// the node value present in between a and b is the lca. Assuming, a<b


Why assume that a < b? Why not ensure it?

havanagrawal · 2018-04-12T19:42:13Z

Session08/binarytree/solutions/BinarySearchTree.java

+	{
+	    if(root == null) {flag=1;return null;}
+
+	    if(root.data>b && root.data>a) lca(root.left,a,b);


If you have assumed that a < b, is it really necessary to check if root.data is greater than both of them?

havanagrawal · 2018-04-12T19:42:20Z

Session08/binarytree/solutions/BinarySearchTree.java

+	    if(root == null) {flag=1;return null;}
+
+	    if(root.data>b && root.data>a) lca(root.left,a,b);
+	    else if(root.data<b && root.data<a) lca(root.right,a,b);


If you have assumed that a < b, is it really necessary to check if root.data is lesser than both of them?

Showndarya · 2018-04-17T13:47:32Z

Thank you for the review.

I have taken all the improvements and suggestions into consideration and have come up with a more refined solution.

Let me know if more changes are required.

Add solution for BinarySearchTree

3d83e8a

Showndarya mentioned this pull request Apr 8, 2018

Add solutions for Binary Search Tree and Linked List #7

Open

havanagrawal reviewed Apr 12, 2018

View reviewed changes

Updated solution for Binary Search Tree

adb53ff

Tweaking in code style

985c85a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add solution for BinarySearchTree #11

Add solution for BinarySearchTree #11

Showndarya commented Apr 8, 2018 •

edited

Loading

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018 •

edited

Loading

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018 •

edited

Loading

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal commented Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

havanagrawal Apr 12, 2018

Showndarya commented Apr 17, 2018 •

edited

Loading


		class BinarySearchTree extends BinaryTree {

		private int flag = 0;

		@@ -0,0 +1,123 @@
		// This class fails to compile, can you figure out why, and fix it?
		// This is beacuse BinaryTree has a constructor which is not default. Hence, when we extend this class and try to call a no-argument constructor, it'll give a compile-time error. so, we call it by passing the parameters (to instantiate the root node) in super() called inside the constructor of BinarySearchTree

Add solution for BinarySearchTree #11

Are you sure you want to change the base?

Add solution for BinarySearchTree #11

Conversation

Showndarya commented Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havanagrawal Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havanagrawal Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havanagrawal commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Showndarya commented Apr 17, 2018 • edited Loading

Showndarya commented Apr 8, 2018 •

edited

Loading

havanagrawal Apr 12, 2018 •

edited

Loading

havanagrawal Apr 12, 2018 •

edited

Loading

Showndarya commented Apr 17, 2018 •

edited

Loading